Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let #[allow(unstable_name_collisions)] work for things other than function #81922

Merged
merged 3 commits into from
Apr 5, 2021

Conversation

magurotuna
Copy link
Contributor

@magurotuna magurotuna commented Feb 9, 2021

Fixes #81522

In addition to the report in #81522, currently #[allow(unstable_name_collisions)] doesn't suppress the corresponding diagnostics even if this attribute is appended to an expression statement or a let statement. It seems like this is because the wrong HirId is passed to struct_span_lint_hir.
It's fixed in this PR, and a regression test for it is also added.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2021
@rust-log-analyzer

This comment has been minimized.

@magurotuna magurotuna marked this pull request as draft February 9, 2021 13:21
@magurotuna
Copy link
Contributor Author

magurotuna commented Feb 9, 2021

Hmm, according to the CI, my fix doesn't work for macro invocations (so I made this PR drafted for now). But perhaps is it expected behavior? In fact, take the following as an example:

macro_rules! foo {
    ($i:ident) => {
        let $i = 42;
    };
}

fn main() {
    #[allow(unused_variables)]
    foo!(a);
}

#[allow(unused_variables)] in the above example doesn't suppress the lint diagnostic:

warning: unused variable: `a`
 --> src/main.rs:3:13
  |
3 |         let $i = 42;
  |             ^^ help: if this is intentional, prefix it with an underscore: `_a`
...
9 |     foo!(a);
  |     -------- in this macro invocation
  |
  = note: `#[warn(unused_variables)]` on by default
  = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: 1 warning emitted

However, if the attribute is appended to main function like:

macro_rules! foo {
    ($i:ident) => {
        let $i = 42;
    };
}

#[allow(unused_variables)]
fn main() {
    foo!(a);
}

then it does suppress the lint!
So I'm wondering if I should take macro invocations into account in this PR or not... 😕


For now, I just deleted a test for macro invocation from issue-81522.rs. Please let me know if macro invocations should also be covered.

@magurotuna magurotuna marked this pull request as ready for review February 9, 2021 15:37
@@ -1511,6 +1519,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
self.orig_steps_var_values.clone(),
steps,
IsSuggestion(true),
scope_expr_id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be self.scope_expr_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct! Fixed it.

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2021

📌 Commit 06b3636 has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81922 (Let `#[allow(unstable_name_collisions)]` work for things other than function)
 - rust-lang#82483 (Use FromStr trait for number option parsing)
 - rust-lang#82739 (Use the beta compiler for building bootstrap tools when `download-rustc` is set)
 - rust-lang#83650 (Update Source Serif to release 4.004)
 - rust-lang#83826 (List trait impls before deref methods in doc's sidebar)
 - rust-lang#83831 (Add `#[inline]` to IpAddr methods)
 - rust-lang#83863 (Render destructured struct function param names as underscore)
 - rust-lang#83865 (Don't report disambiguator error if link would have been ignored)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Apr 5, 2021

⌛ Testing commit 06b3636 with merge 39eee17...

@bors bors merged commit 54ea8e1 into rust-lang:master Apr 5, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 5, 2021
@magurotuna magurotuna deleted the issue81522 branch April 5, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[allow(unstable_name_collisions)] doesn't work at block scope
6 participants